fix: recover from browser context crash without server restart#385
fix: recover from browser context crash without server restart#385whyyagswhy wants to merge 1 commit intostickerdaniel:mainfrom
Conversation
…dleware When the Patchright browser context dies mid-operation (TargetClosedError), the HTTP server stays alive and all subsequent tool calls fail with the same error. The fix detects this condition in the middleware — the single chokepoint for all tool calls — resets the browser singleton, and returns a descriptive ToolError so the client knows to retry. The next call reinitializes the browser from the persistent on-disk profile without requiring a new LinkedIn login. Before this fix: every tool call after a browser crash returns TargetClosedError indefinitely until the server process is manually restarted. After this fix: the crashing call returns a clear "please retry" message; the next call reinitializes the browser and succeeds (~5–10s cold start). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR adds automatic recovery from Confidence Score: 5/5Safe to merge — recovery logic is correct and all remaining findings are P2 style suggestions. The singleton is reset before any potentially-failing browser.close() call, so recovery is guaranteed regardless of whether close_browser() raises. Both open comments are hardening/style suggestions, not correctness defects. No files require special attention; sequential_tool_middleware.py is the only changed file and its logic is correct. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as SequentialToolMiddleware
participant Tool as Tool Handler
participant Browser as BrowserManager
Client->>Middleware: on_call_tool(context)
Middleware->>Middleware: acquire _lock
Middleware->>Tool: call_next(context)
Tool->>Browser: page operation
Browser-->>Tool: TargetClosedError
Tool-->>Middleware: raises TargetClosedError
alt _is_browser_context_closed == True
Middleware->>Middleware: log WARNING
Middleware->>Browser: close_browser() sets _browser=None
Middleware-->>Client: raise ToolError please retry
else other exception
Middleware-->>Client: re-raise original exception
end
Client->>Middleware: retry on_call_tool(context)
Middleware->>Browser: get_or_create_browser() reinitialises from disk
Browser-->>Middleware: fresh BrowserManager
Middleware->>Tool: call_next(context)
Tool-->>Client: success
Prompt To Fix All With AIThis is a comment left during a code review.
Path: linkedin_mcp_server/sequential_tool_middleware.py
Line: 85-93
Comment:
**`close_browser()` may itself raise on a crashed context**
`close_browser()` resets the singleton first (safe), but then calls `await browser.close()` without a surrounding `try/except`. When the browser context is already dead, that call is likely to raise another Playwright error, which propagates out of `close_browser()` and is silently swallowed here at `DEBUG` level. Recovery still works because the singleton was already cleared, but the only observable signal is a DEBUG log rather than a WARNING. Consider logging at `WARNING` when the outer catch fires, or wrapping `browser.close()` inside `close_browser()` itself so the crash path is explicit.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: linkedin_mcp_server/sequential_tool_middleware.py
Line: 22-24
Comment:
**Substring match may miss localized or wrapped error messages**
`_is_browser_context_closed` converts the exception to a string and checks for a substring. If the Playwright / Patchright version in use ever wraps `TargetClosedError` inside another exception type (e.g. a transport or serialization wrapper), `str(exc)` may only contain the outer exception's message and the match will silently fail, leaving the server in the broken-singleton state. Checking `type(exc).__name__ == "TargetClosedError"` (or a similar type-based guard) as a primary condition, with the substring as a fallback, would be more robust.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: recover from browser context crash ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| # Lazy import avoids a circular dependency at module load time. | ||
| from linkedin_mcp_server.drivers.browser import close_browser | ||
| await close_browser() | ||
| except Exception: | ||
| logger.debug( | ||
| "close_browser() failed during crash recovery", | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
close_browser() may itself raise on a crashed context
close_browser() resets the singleton first (safe), but then calls await browser.close() without a surrounding try/except. When the browser context is already dead, that call is likely to raise another Playwright error, which propagates out of close_browser() and is silently swallowed here at DEBUG level. Recovery still works because the singleton was already cleared, but the only observable signal is a DEBUG log rather than a WARNING. Consider logging at WARNING when the outer catch fires, or wrapping browser.close() inside close_browser() itself so the crash path is explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: linkedin_mcp_server/sequential_tool_middleware.py
Line: 85-93
Comment:
**`close_browser()` may itself raise on a crashed context**
`close_browser()` resets the singleton first (safe), but then calls `await browser.close()` without a surrounding `try/except`. When the browser context is already dead, that call is likely to raise another Playwright error, which propagates out of `close_browser()` and is silently swallowed here at `DEBUG` level. Recovery still works because the singleton was already cleared, but the only observable signal is a DEBUG log rather than a WARNING. Consider logging at `WARNING` when the outer catch fires, or wrapping `browser.close()` inside `close_browser()` itself so the crash path is explicit.
How can I resolve this? If you propose a fix, please make it concise.| def _is_browser_context_closed(exc: Exception) -> bool: | ||
| """Return True if *exc* indicates the Patchright browser context has died.""" | ||
| return _BROWSER_CONTEXT_CLOSED in str(exc) |
There was a problem hiding this comment.
_is_browser_context_closed converts the exception to a string and checks for a substring. If the Playwright / Patchright version in use ever wraps TargetClosedError inside another exception type (e.g. a transport or serialization wrapper), str(exc) may only contain the outer exception's message and the match will silently fail, leaving the server in the broken-singleton state. Checking type(exc).__name__ == "TargetClosedError" (or a similar type-based guard) as a primary condition, with the substring as a fallback, would be more robust.
Prompt To Fix With AI
This is a comment left during a code review.
Path: linkedin_mcp_server/sequential_tool_middleware.py
Line: 22-24
Comment:
**Substring match may miss localized or wrapped error messages**
`_is_browser_context_closed` converts the exception to a string and checks for a substring. If the Playwright / Patchright version in use ever wraps `TargetClosedError` inside another exception type (e.g. a transport or serialization wrapper), `str(exc)` may only contain the outer exception's message and the match will silently fail, leaving the server in the broken-singleton state. Checking `type(exc).__name__ == "TargetClosedError"` (or a similar type-based guard) as a primary condition, with the substring as a fallback, would be more robust.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Problem
When the Patchright browser context dies mid-operation, the MCP HTTP server
stays alive but every subsequent tool call fails with:
Because the
_browsersingleton is still set (to the dead context),get_or_create_browser()keeps returning it. The server appears healthy toexternal monitors (HTTP 200) but cannot actually perform any LinkedIn operations.
The only recovery is a manual process restart.
Root cause
SequentialToolExecutionMiddleware.on_call_tool()is the single chokepoint forall tool calls, but it has no handling for browser-level errors — only a
finallyblock for timing.
TargetClosedErroris also absent fromerror_handler.py.Fix
In
on_call_tool():TargetClosedError(matched by the Playwright error message substring,which is stable across versions).
close_browser()to reset the_browsersingleton toNone.ToolErrorwith a clear "please retry" message.On the next tool call,
get_or_create_browser()finds_browser is Noneandreinitializes from the persistent on-disk profile — no LinkedIn re-login
required, since cookies are stored in the profile directory.
Behaviour after this fix
Test
Tested manually: caused a browser context crash (via
close_sessiontoolmid-operation), confirmed the next tool call recovered automatically.